Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't overbuild source generator when root building for net48 #105347

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jul 23, 2024

Fixes #105364

Just noticed that source generators are now built twice. Regressed with e2a3511

image

Adding this blurb as in oob-all, oob-ref and oob-src.proj fixes that.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

<PropertyGroup Condition="'$(BuildTargetFramework)' != '' and '$(BuildTargetFramework)' == '$(NetFrameworkCurrent)'">
<TargetFramework>$(BuildTargetFramework)</TargetFramework>
<!-- Filter ProjectReferences to build the best matching target framework only. -->
<FilterTraversalProjectReferences>true</FilterTraversalProjectReferences>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this still overbuild in the case that a generator doesn't multi-target? What was causing the overbuild to happen? I would have expected any duplicates to hit the MSBuild project cache - were global properties different?

Copy link
Member Author

@ViktorHofer ViktorHofer Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different global properties, yes. More precisely, the BuildTargetFramework property set.

This solution works for both multi targeting and non multi-targeting builds and is just following what we already do in the other oob-*.projs. I simpy forgot to add that part.

See https://github.com/dotnet/runtime/blob/main/eng%2FTraversalSdk.AfterTargets.targets

@ViktorHofer
Copy link
Member Author

/ba-g CI pipeline is stuck (GH thinks it still runs but it already canceled on AzDO side)

@ViktorHofer
Copy link
Member Author

@ericstj I'm going ahead merging this as the failure is observable in CI already. I'm happy to react to any feedback in a follow-up.

@ViktorHofer ViktorHofer merged commit a47eeec into main Jul 24, 2024
81 of 84 checks passed
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-2 branch July 24, 2024 08:25
@ericstj
Copy link
Member

ericstj commented Jul 24, 2024

I see - I am better understanding this change now. Before I was worried we'd force a build with the TargetFramework value of BuildTargetFramework, but I see that instead you're evaluating the child projects for that framework.

I wonder if this should actually be the default for all Traversal projects when BuildTargetFramework is set.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to copy files during CI build
3 participants